Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create runtime fields #622

Merged
merged 13 commits into from
May 16, 2024
Merged

Create runtime fields #622

merged 13 commits into from
May 16, 2024

Conversation

vim345
Copy link
Contributor

@vim345 vim345 commented Feb 22, 2024

The following query works now. It supports all the numeric values, strings, maps and lists.

{
  "indexName": "testRuntime",
  "startHit": 0,
  "topHits": 5,
  "retrieveFields": [
    "vendor_name",
    "license_no",
    "v1",
    "r1",
    "r2",
    "r3",
    "r4",
    "r5",
    "r6"
  ],
  "runtimeFields": [
    {
      "name": "r1",
      "script": {
        "lang": "painless",
        "source": "return Collections.singletonMap('key', 2.5);"
      }
    },
    {
      "name": "r2",
      "script": {
        "lang": "painless",
        "source": "return false;"
      }
    },
    {
      "name": "r3",
      "script": {
        "lang": "painless",
        "source": "Map map = Collections.singletonMap('internalKey', 2.5); return Collections.singletonMap('key', map);"
      }
    },
    {
      "name": "r4",
      "script": {
        "lang": "painless",
        "source": "List nums = new ArrayList(); nums.add('1'); nums.add('2'); return nums;"
      }
    },
    {
      "name": "r5",
      "script": {
        "lang": "painless",
        "source": "List nums = new ArrayList(); nums.add(1); nums.add(2); return nums;"
      }
    },
    {
      "name": "r6",
      "script": {
        "lang": "painless",
        "source": "Map map = Collections.singletonMap('key', 2.5); List maps = new ArrayList(); maps.add(map); maps.add(map); return maps;"
      }
    }
  ],
  "virtualFields": [
    {
      "name": "v1",
      "script": {
        "lang": "painless",
        "source": "return 2 * 4.0;"
      }
    }
  ]
}

Which gave me the following:

[INFO ] 2024-03-18 15:49:57.012 [main] LuceneServerClientBuilder$SearchClientBuilder - Converting fields {
  "indexName": "testRuntime",
  "startHit": 0,
  "topHits": 5,
  "retrieveFields": [
    "vendor_name",
    "license_no",
    "v1",
    "r1",
    "r2",
    "r3",
    "r4",
    "r5",
    "r6"
  ],
  "runtimeFields": [
    {
      "name": "r1",
      "script": {
        "lang": "painless",
        "source": "return Collections.singletonMap('key', 2.5);"
      }
    },
    {
      "name": "r2",
      "script": {
        "lang": "painless",
        "source": "return false;"
      }
    },
    {
      "name": "r3",
      "script": {
        "lang": "painless",
        "source": "Map map = Collections.singletonMap('internalKey', 2.5); return Collections.singletonMap('key', map);"
      }
    },
    {
      "name": "r4",
      "script": {
        "lang": "painless",
        "source": "List nums = new ArrayList(); nums.add('1'); nums.add('2'); return nums;"
      }
    },
    {
      "name": "r5",
      "script": {
        "lang": "painless",
        "source": "List nums = new ArrayList(); nums.add(1); nums.add(2); return nums;"
      }
    },
    {
      "name": "r6",
      "script": {
        "lang": "painless",
        "source": "Map map = Collections.singletonMap('key', 2.5); List maps = new ArrayList(); maps.add(map); maps.add(map); return maps;"
      }
    }
  ],
  "virtualFields": [
    {
      "name": "v1",
      "script": {
        "lang": "painless",
        "source": "return 2 * 4.0;"
      }
    }
  ]
}
 to proto SearchRequest
[INFO ] 2024-03-18 15:49:57.093 [main] LuceneServerClientBuilder$SearchClientBuilder - jsonStr converted to proto SearchRequest: 
indexName: "testRuntime"
topHits: 5
retrieveFields: "vendor_name"
retrieveFields: "license_no"
retrieveFields: "v1"
retrieveFields: "r1"
retrieveFields: "r2"
retrieveFields: "r3"
retrieveFields: "r4"
retrieveFields: "r5"
retrieveFields: "r6"
virtualFields {
  script {
    lang: "painless"
    source: "return 2 * 4.0;"
  }
  name: "v1"
}
runtimeFields {
  script {
    lang: "painless"
    source: "return Collections.singletonMap(\'key\', 2.5);"
  }
  name: "r1"
}
runtimeFields {
  script {
    lang: "painless"
    source: "return false;"
  }
  name: "r2"
}
runtimeFields {
  script {
    lang: "painless"
    source: "Map map = Collections.singletonMap(\'internalKey\', 2.5); return Collections.singletonMap(\'key\', map);"
  }
  name: "r3"
}
runtimeFields {
  script {
    lang: "painless"
    source: "List nums = new ArrayList(); nums.add(\'1\'); nums.add(\'2\'); return nums;"
  }
  name: "r4"
}
runtimeFields {
  script {
    lang: "painless"
    source: "List nums = new ArrayList(); nums.add(1); nums.add(2); return nums;"
  }
  name: "r5"
}
runtimeFields {
  script {
    lang: "painless"
    source: "Map map = Collections.singletonMap(\'key\', 2.5); List maps = new ArrayList(); maps.add(map); maps.add(map); return maps;"
  }
  name: "r6"
}

[INFO ] 2024-03-18 15:49:57.219 [main] LuceneServerClient - Server returned : diagnostics {
  firstPassSearchTimeMs: 1.395666
  getFieldsTimeMs: 7.184209
}
totalHits {
  value: 2
}
hits {
  score: 1.0
  fields {
    key: "license_no"
    value {
      fieldValue {
        intValue: 100
      }
      fieldValue {
        intValue: 200
      }
    }
  }
  fields {
    key: "r1"
    value {
      fieldValue {
        structValue {
          fields {
            key: "key"
            value {
              number_value: 2.5
            }
          }
        }
      }
    }
  }
  fields {
    key: "r2"
    value {
      fieldValue {
        booleanValue: false
      }
    }
  }
  fields {
    key: "r3"
    value {
      fieldValue {
        structValue {
          fields {
            key: "key"
            value {
              struct_value {
                fields {
                  key: "internalKey"
                  value {
                    number_value: 2.5
                  }
                }
              }
            }
          }
        }
      }
    }
  }
  fields {
    key: "r4"
    value {
      fieldValue {
        repeatedFieldValues {
          textValues: "1"
          textValues: "2"
        }
      }
    }
  }
  fields {
    key: "r5"
    value {
      fieldValue {
        repeatedFieldValues {
          intValues: 1
          intValues: 2
        }
      }
    }
  }
  fields {
    key: "r6"
    value {
      fieldValue {
        repeatedFieldValues {
          structValues {
            fields {
              key: "key"
              value {
                number_value: 2.5
              }
            }
          }
          structValues {
            fields {
              key: "key"
              value {
                number_value: 2.5
              }
            }
          }
        }
      }
    }
  }
  fields {
    key: "v1"
    value {
      fieldValue {
        doubleValue: 8.0
      }
    }
  }
  fields {
    key: "vendor_name"
    value {
      fieldValue {
        textValue: "first vendor"
      }
    }
  }
}
hits {
  luceneDocId: 1
  score: 1.0
  fields {
    key: "license_no"
    value {
      fieldValue {
        intValue: 111
      }
      fieldValue {
        intValue: 222
      }
    }
  }
  fields {
    key: "r1"
    value {
      fieldValue {
        structValue {
          fields {
            key: "key"
            value {
              number_value: 2.5
            }
          }
        }
      }
    }
  }
  fields {
    key: "r2"
    value {
      fieldValue {
        booleanValue: false
      }
    }
  }
  fields {
    key: "r3"
    value {
      fieldValue {
        structValue {
          fields {
            key: "key"
            value {
              struct_value {
                fields {
                  key: "internalKey"
                  value {
                    number_value: 2.5
                  }
                }
              }
            }
          }
        }
      }
    }
  }
  fields {
    key: "r4"
    value {
      fieldValue {
        repeatedFieldValues {
          textValues: "1"
          textValues: "2"
        }
      }
    }
  }
  fields {
    key: "r5"
    value {
      fieldValue {
        repeatedFieldValues {
          intValues: 1
          intValues: 2
        }
      }
    }
  }
  fields {
    key: "r6"
    value {
      fieldValue {
        repeatedFieldValues {
          structValues {
            fields {
              key: "key"
              value {
                number_value: 2.5
              }
            }
          }
          structValues {
            fields {
              key: "key"
              value {
                number_value: 2.5
              }
            }
          }
        }
      }
    }
  }
  fields {
    key: "v1"
    value {
      fieldValue {
        doubleValue: 8.0
      }
    }
  }
  fields {
    key: "vendor_name"
    value {
      fieldValue {
        textValue: "second vendor"
      }
    }
  }
}
searchState {
  timestamp: 1710802197
  searcherVersion: 4
  lastDocId: 1
  lastScore: 1.0
}

@vim345 vim345 requested a review from aprudhomme February 22, 2024 22:41
@vim345 vim345 force-pushed the u/mohm/RP-9936_runtime branch from 5d4d3ee to 80b3f22 Compare February 22, 2024 22:42
clientlib/src/main/proto/yelp/nrtsearch/search.proto Outdated Show resolved Hide resolved
clientlib/src/main/proto/yelp/nrtsearch/search.proto Outdated Show resolved Hide resolved
* @param docLookup index level doc value lookup provider
* @return {@link ValueSource} to evaluate script
*/
ValueSource newFactory(Map<String, Object> params, DocLookup docLookup);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure you are really getting much using ValueSource. SegmentFactory could be a simple interface.

The RuntimeFieldDef could just provide a RuntimeScript.SegmentFactory implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Converting to SegmentFactory was much simpler and more straightforward.

compositeFieldValue.addFieldValue(
SearchResponse.Hit.FieldValue.newBuilder().setTextValue(String.valueOf(obj)));
}
if (obj instanceof Long) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will probably need to add some more types here. Converting a Map into a Struct would be an important one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to do map, but got some errors converting a map to proto object.

I added a TODO to add map and list.

@vim345 vim345 force-pushed the u/mohm/RP-9936_runtime branch 3 times, most recently from 88fe12c to fca27c0 Compare March 14, 2024 00:44
@vim345
Copy link
Contributor Author

vim345 commented Mar 14, 2024

@aprudhomme thank you for reviewing this. I applied your suggestions. Can you please take a look at this to see if the overall design is good enough so that I can create tests for this PR? I will try to support map and list types with the test commit .

Copy link
Contributor

@aprudhomme aprudhomme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a reasonable approach

@vim345 vim345 force-pushed the u/mohm/RP-9936_runtime branch 2 times, most recently from c8ff362 to 767903d Compare March 18, 2024 23:41
@vim345 vim345 force-pushed the u/mohm/RP-9936_runtime branch from 9e4dca0 to 86eda24 Compare March 20, 2024 23:53
@vim345
Copy link
Contributor Author

vim345 commented Mar 21, 2024

@aprudhomme can you please take a look at this? I think the failing tests are flaky ones that don't have anything to do with my changes. I

@vim345 vim345 force-pushed the u/mohm/RP-9936_runtime branch from 1521755 to 8a04251 Compare March 21, 2024 22:38
@@ -598,12 +608,30 @@ message SearchResponse {
google.protobuf.Struct structValue = 8; // Value for structured data
// Value for VECTOR FieldType
Vector vectorValue = 9;
RepeatedFieldValues repeatedFieldValues = 10;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointer. I spent a lot of time creating this and its builder methods. Using ListValue and the existing struct builders would have saved me a lot of time, if I had seen them earlier.

@@ -763,7 +768,6 @@ private CompositeFieldValue getFieldForHit(

// We detect invalid field above:
assert fd != null;

if (fd instanceof VirtualFieldDef) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the runtime field also need to be covered in this code path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow the code never comes here when I put a breakpoint in this line. I assume it's not necessary to do that anymore. It doesn't stop here, even when I have a virtual field in the request.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code path is used for doing parallel fetch by chunks of fields, instead of documents. It should probably be supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know how I can test that locally?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I was able to debug this part of the code with the above configs. It now returns the same object as the one with fetching docs.

@@ -91,11 +91,14 @@ public static UpdatedFieldInfo updateFields(
FieldAndFacetState.Builder fieldStateBuilder = currentState.toBuilder();
List<Field> nonVirtualFields = new ArrayList<>();
List<Field> virtualFields = new ArrayList<>();
List<Field> runtimeFields = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it wasn't used. So I removed it.

@vim345 vim345 force-pushed the u/mohm/RP-9936_runtime branch from 4f3bb8b to 5dd576a Compare March 22, 2024 22:35
@vim345
Copy link
Contributor Author

vim345 commented May 15, 2024

@aprudhomme can you take a look at this when you get a chance?

@@ -763,7 +768,6 @@ private CompositeFieldValue getFieldForHit(

// We detect invalid field above:
assert fd != null;

if (fd instanceof VirtualFieldDef) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code path is used for doing parallel fetch by chunks of fields, instead of documents. It should probably be supported.

@vim345 vim345 merged commit 1f0071e into Yelp:master May 16, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants